Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tab completion and syntax checking #261

Merged
merged 66 commits into from
Oct 24, 2024

Conversation

kernelPanic0x
Copy link
Contributor

@kernelPanic0x kernelPanic0x commented Sep 23, 2024

A really nice feature I would love is TAB completion for the wormhole codes. This is a fully functional and working prototype with the reedline crate, which provides most of the functionality already. I also decided to use fuzzy string searching using the Jaro-Winkler algorithm to autocomplete words containing spelling mistakes, especially for people with poor English language knowledge via verbal code propagation.

TODO list:

  • I needed to make the core::wordlist module public to access the wordlist in CLI. Some documentation is still missing there.
  • Better CTRL-C handling maybe? I just bail out with "Ctrl-C received" but perhaps this can be done more elegantly. It shouldn't be the normal case anyway.

Feedback would be appreciated. Let's make magick-wormhole.rs catch up with the og 😄

PS: i think this needs to be squashed, had some trouble merging with upstream after creating branch...

@kernelPanic0x
Copy link
Contributor Author

I've tried it out, some feedback on the usability:

  • Why does it show a green | character in front of the input sometimes? It is rather confusing
  • The entered code is always red, even though it is totally valid. Arguably this shouldn't even do much validation itself, as people might be using their own wordlists.
  • NO RECORDS FOUND is inaccurate. It also appears before even typing any characters. Maybe that message can be removed?

I think the bar idicates that the selection menu is active? The code should turn green when it has a number and at least two valid words in it. Yes "NO RECORDS FOUND" is not the ideal wording here. I have not looking into docs on how i can change this. I will see what we can do about it.

Comment on lines 106 to 112
pub fn default_wordlist_flatned() -> Vec<String> {
load_pgpwords()
.iter()
.flatten()
.map(|s| s.clone())
.collect()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think flattening is the right approach here. Which word is chosen depends on the current word index.

See https://github.com/magic-wormhole/magic-wormhole.rs/blob/main/src/core/wordlist.rs#L24 as for how it works. The list of possible words that is chosen depends on whether the word is at an even or odd position.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah i see. That makes sense! Will look into this further...

@kernelPanic0x
Copy link
Contributor Author

kernelPanic0x commented Oct 7, 2024

I am currently thinking about how we can refactor the completion part into wordlist. I get problems with reedline because reedline uses also the pos variable and not the whole line. Either we just complete the whole line step by step each time, or we use pos and complete each word independently depending on pos like it is now. Which approach would be the best? Probably the full line complete like the get_completions fn we've already got right? Need to test to find out if this works. Right now the user is able to move the cursor back and change a word in the middle. But i think this is not really needed right?

@felinira
Copy link
Collaborator

felinira commented Oct 7, 2024

Right now the user is able to move the cursor back and change a word in the middle. But i think this is not really needed right?

I find it rather unrealistic that one would need to do such a thing.

However I don't really care much, you can add an optional pos parameter too if you feel like it. Something like

pub fn get_completions(&self, prefix: &str, pos: Option<usize>) -> Vec<String>;

where if pos is not set, it will use the prefix len?

Alternatively you can just return no completions to reedline if pos != len - 1 and use the existing signature.

If you add additional dependencies to the library crate for completions, please add them behind a feature flag (could call it completion) :)

@kernelPanic0x
Copy link
Contributor Author

kernelPanic0x commented Oct 9, 2024

I've switched to dialoguer crate. Keep it simple. But i think i found a little bug in dialoguer crate. When i move the cursor to the left in a word and tab complete it messes up the prefix text somehow. I think i need to open a bug report in dialoguer repo for that 👀.

And what am i missing with the fuzzy-complete feature with our tests? Classic case of it worked on my machine 🤣...

@kernelPanic0x
Copy link
Contributor Author

kernelPanic0x commented Oct 10, 2024

@felinira When i run cargo test --no-default-features --package magic-wormhole i get a pretty ugly error message from eyre:

error[E0432]: unresolved import `crate::transit`
  --> src\core\test.rs:9:5
   |
9  |     transit, AppConfig, AppID, Code, Wormhole, WormholeError,
   |     ^^^^^^^ no `transit` in the root
   |
note: found an item that was configured out
  --> src\lib.rs:37:9
   |
37 | pub mod transit;
   |         ^^^^^^^
note: the item is gated behind the `transit` feature
  --> src\lib.rs:36:7
   |
36 | #[cfg(feature = "transit")]
   |       ^^^^^^^^^^^^^^^^^^^

error[E0433]: failed to resolve: could not find `transit` in the crate root
  --> src\core\test.rs:46:51
   |
46 | pub(crate) fn log_transit_connection(info: crate::transit::TransitInfo) {
   |                                                   ^^^^^^^ could not find `transit` in the crate root
   |
note: found an item that was configured out
  --> src\lib.rs:37:9
   |
37 | pub mod transit;
   |         ^^^^^^^
note: the item is gated behind the `transit` feature
  --> src\lib.rs:36:7
   |
36 | #[cfg(feature = "transit")]
   |       ^^^^^^^^^^^^^^^^^^^

error[E0433]: failed to resolve: use of undeclared crate or module `transfer`
  --> src\core\test.rs:96:24
   |
96 | ) -> eyre::Result<Vec<(transfer::offer::OfferSend, transfer::offer::OfferAccept)>> {
   |                        ^^^^^^^^ use of undeclared crate or module `transfer`

error[E0433]: failed to resolve: use of undeclared crate or module `transfer`
  --> src\core\test.rs:96:52
   |
96 | ) -> eyre::Result<Vec<(transfer::offer::OfferSend, transfer::offer::OfferAccept)>> {
   |                                                    ^^^^^^^^ use of undeclared crate or module `transfer`

error[E0433]: failed to resolve: use of undeclared crate or module `transfer`
  --> src\core\test.rs:99:24
   |
99 |     ) -> eyre::Result<(transfer::offer::OfferSend, transfer::offer::OfferAccept)> {
   |                        ^^^^^^^^ use of undeclared crate or module `transfer`
...

Do we need to rethink how we use the default features in magic-wormhole package for the whole conditional fuzzy-complete feature to be variable with no-default-features? Maybe it would be better to use no-fuzzy-complete as a feature instead of fuzzy-complete inside default.

@felinira
Copy link
Collaborator

Do we need to rethink how we use the default features in magic-wormhole package

Yeah, that's on my list in my head for the next breaking change which would be 0.8. The entire crate needs a spring cleaning, which is easier once we have removed all deprecated items. I would however prefer to not do another breaking release for a little while longer, as it's always more work than anticipated. I don't have much time for it until EOY at least.

The empty feature build should also be properly tested in CI. But as this also occurs in main I would consider it out of scope for this PR. But feel free to submit a PR, I suppose we need to branch 0.8 eventually.

@felinira
Copy link
Collaborator

I will get #257 in first, because I would consider it a security fix, I hope it doesn't create too many conflicts.

@felinira felinira merged commit 83cbe0e into magic-wormhole:main Oct 24, 2024
20 checks passed
@felinira
Copy link
Collaborator

Thanks a lot, it works well for now, at least for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants